Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 3, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Ready Ready Preview Comment Nov 4, 2025 8:26pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 4, 2025 8:26pm
rivet-inspector Ignored Ignored Preview Nov 4, 2025 8:26pm
rivet-site Ignored Ignored Preview Nov 4, 2025 8:26pm

Copy link
Member Author

NathanFlurry commented Nov 3, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 3, 2025

More templates

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/actor@3312

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3312

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/core@3312

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3312

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3312

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3312

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3312

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3312

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3312

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3312

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3312

commit: 84e9dc6

@claude
Copy link

claude bot commented Nov 3, 2025

PR Review: Fix ArrayBuffer.prototype.byteLength Error

Summary

This PR updates the on-change dependency from version 5.0.1 to a forked version @rivetkit/[email protected] to fix the error: "Method get ArrayBuffer.prototype.byteLength called on incompatible receiver #".

Code Quality ✅

Positive aspects:

  • Clean, minimal change that addresses a specific runtime error
  • Uses npm package aliasing (npm:@rivetkit/on-change@^6.0.2-rc.1) which is a valid approach for testing forked packages
  • Lock file updates are consistent and appear correct

Concerns & Questions

1. Fork vs. Upstream ⚠️

The change uses a forked version (@rivetkit/on-change) instead of the official on-change package. Key questions:

  • Why was a fork necessary? Is this a temporary fix while waiting for an upstream merge?
  • Has a PR been submitted to the upstream on-change repository?
  • What are the differences between the fork and upstream v6.0.2?
  • Is there a plan to migrate back to the official package once the fix is available upstream?

2. Use of Release Candidate ⚠️

The version being used is 6.0.2-rc.1 (a release candidate):

  • Is this stable enough for production use?
  • What testing has been done to verify this fixes the ArrayBuffer issue without introducing regressions?
  • Consider using a stable release once available

3. Missing Context 📝

The PR body is empty and the commit message is truncated. It would be helpful to include:

  • Steps to reproduce the original error
  • Link to the issue this fixes (if any)
  • Information about what changed between v5.0.1 and v6.0.2-rc.1
  • Testing that was performed to verify the fix

4. Test Coverage

  • Are there tests that verify the ArrayBuffer issue is resolved?
  • Does the existing test suite pass with this change?
  • Consider adding a regression test if one doesn't exist

5. Dependency Management 💡

Consider documenting this fork in the repository:

  • Add a comment in package.json explaining why the fork is used
  • Document in CLAUDE.md or a DEPENDENCIES.md file if this is a long-term fork
  • Track this as a technical debt item if planning to migrate back to upstream

Security Considerations 🔒

  • Using a forked/custom package introduces supply chain risk
  • Ensure the @rivetkit/on-change package is published from a trusted source
  • Verify the package contents match expected changes
  • Consider pinning to an exact version rather than using ^ for a fork

Performance Considerations

No performance concerns identified - this is a dependency version update.

Recommendations

  1. Add PR description with:

    • Context about the error
    • Why a fork was necessary
    • Timeline for migrating to upstream (if applicable)
  2. Documentation: Add an inline comment in package.json:

    // Using forked version to fix ArrayBuffer.prototype.byteLength error
    // TODO: Migrate to upstream once fix is merged
    "on-change": "npm:@rivetkit/on-change@^6.0.2-rc.1",
  3. Testing: Verify that:

    • All existing tests pass
    • The specific ArrayBuffer error no longer occurs
    • No regressions in state change tracking functionality
  4. Follow-up: Create a tracking issue for:

    • Submitting a PR to upstream on-change (if not already done)
    • Migrating back to the official package once available

Verdict

Conditionally Approve ✅ (with reservations)

The change appears technically sound for addressing the immediate issue, but lacks context and documentation. Before merging, please:

  1. Add a description to the PR explaining the change
  2. Confirm testing has been performed
  3. Clarify the long-term plan for this dependency

Review performed following repository conventions from CLAUDE.md

@NathanFlurry NathanFlurry force-pushed the 11-02-chore_scripts_add_engine_shell_command branch from 2ec6506 to ad122ff Compare November 4, 2025 18:59
@NathanFlurry NathanFlurry force-pushed the 11-02-fix_rivetkit_fix_method_get_arraybuffer.prototype.bytelength_called_on_incompatible_receiver_arraybuffer_ branch from 84486b7 to 308b7c8 Compare November 4, 2025 18:59
@vercel
Copy link

vercel bot commented Nov 4, 2025

Deployment failed with the following error:

Too many requests - try again in 5 minutes (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

@claude
Copy link

claude bot commented Nov 4, 2025

PR Review

Summary

This PR updates the on-change dependency from v5.0.1 to a custom fork @rivetkit/[email protected] to fix an ArrayBuffer compatibility error.

Code Quality ✅

Positive aspects:

  • The change is minimal and focused on fixing a specific runtime error
  • Uses a scoped package (@rivetkit/on-change) which suggests this is a controlled fork
  • The pnpm lockfile updates are clean and show proper dependency resolution

Issues & Concerns

1. Documentation Missing ⚠️

  • The PR description is empty - it should explain:
    • What was the exact error message and when it occurred?
    • What change was made in the fork to fix the issue?
    • Why use a fork instead of waiting for an upstream fix or reporting the issue?
    • Is this a temporary workaround or permanent solution?

2. Fork Maintenance Risk ⚠️

  • Using a custom fork (@rivetkit/on-change) introduces maintenance overhead:
    • Who maintains this fork?
    • What's the plan to sync with upstream?
    • Is there an issue tracker for this fork?
    • Consider documenting the fork's purpose in the codebase

3. Testing Coverage

  • No test changes included to verify the fix
  • Consider adding a regression test that:
    • Uses ArrayBuffers in actor state (which triggers onChange proxy)
    • Verifies the ArrayBuffer.prototype.byteLength error doesn't occur
    • Example location: tests/actor-types.test.ts or tests/driver-memory.test.ts

4. Context for the Error 📝

Based on code analysis, the error likely occurs because:

  • on-change creates proxies around actor state (line 699 in instance.ts)
  • Actor state contains ArrayBuffer instances (used in scheduleEvents, WebSocket requestIds, etc.)
  • v5 of on-change may have issues with ArrayBuffer proxy handlers
  • The fork likely fixes how ArrayBuffer objects are handled in the proxy trap

Security Considerations ✅

  • Using a @rivetkit scoped package suggests internal control
  • Verify the fork source is from a trusted repository
  • Ensure the fork doesn't introduce any malicious code

Performance Considerations ✅

  • No performance impact expected from this dependency update
  • The change only affects the on-change library internals

Recommendations

  1. Document the fix: Add PR description explaining:

    • The exact error encountered
    • Link to the fork repository
    • What specific change was made to fix the issue
  2. Add tests: Create a test case that exercises ArrayBuffer handling in actor state to prevent regression

  3. Upstream tracking:

    • File an issue with the original on-change maintainer if not already done
    • Add a TODO comment in package.json about reverting to upstream once fixed
  4. Version clarity: The version 6.0.2-rc.1 suggests pre-release - document the stability expectations

  5. Alternative consideration: If the ArrayBuffer compatibility is a known issue with on-change v5, consider whether there's a way to avoid storing ArrayBuffers in proxied state, or use a different state management approach for binary data.

Overall Assessment

The fix appears sound for addressing the immediate issue, but lacks documentation and test coverage. The use of a custom fork should be temporary with a clear migration path back to upstream.

Recommendation: Approve with requested changes (documentation + tests).

@NathanFlurry NathanFlurry force-pushed the 11-02-chore_scripts_add_engine_shell_command branch from ad122ff to 87a9fb6 Compare November 4, 2025 19:24
@NathanFlurry NathanFlurry force-pushed the 11-02-fix_rivetkit_fix_method_get_arraybuffer.prototype.bytelength_called_on_incompatible_receiver_arraybuffer_ branch from 308b7c8 to 9d52114 Compare November 4, 2025 19:24
@claude
Copy link

claude bot commented Nov 4, 2025

Pull Request Review

Summary

This PR updates the on-change dependency from the standard npm package (v5.0.1) to a forked version @rivetkit/on-change@^6.0.2-rc.1. The change addresses an ArrayBuffer compatibility error: "Method get ArrayBuffer.prototype.byteLength called on incompatible receiver".

Code Quality & Best Practices

Positive Aspects:

  • The change is minimal and focused on a single dependency update
  • The lockfile changes appear clean with proper transitive dependency updates

Concerns:

  1. Using an RC (Release Candidate) version in production

    • The forked package is versioned as 6.0.2-rc.1, which suggests it's not production-ready
    • Consider when this will be promoted to a stable release
    • Is there a timeline to either:
      • Merge fixes upstream to the official on-change package?
      • Promote the fork to a stable version?
  2. Missing documentation

    • The PR body is empty - it would be helpful to document:
      • What specific scenario triggers the ArrayBuffer error
      • Why a fork was necessary instead of fixing upstream
      • What changes were made in the forked version
      • Whether this is a temporary or permanent fork
  3. Dependency maintenance burden

    • Maintaining a forked dependency increases long-term maintenance costs
    • Need to track both upstream changes and security updates
    • Consider adding a TODO or issue to track this technical debt

Potential Issues

  1. Breaking changes risk: The jump from v5.0.1 to v6.0.2 suggests this is a major version bump. Have you verified:

    • API compatibility with existing usage patterns in actor/instance.ts?
    • No behavioral changes that could affect state change detection?
    • The onChange usage pattern at line 699 still works correctly?
  2. Testing gap:

    • Are there tests that specifically cover the ArrayBuffer scenario that was failing?
    • Consider adding a regression test to prevent this issue from reoccurring

Security Concerns

  • Using a forked package from the @rivetkit scope appears safe since it's under your organization's control
  • However, ensure the forked repository is actively monitored for security issues
  • Document the fork's location for the team

Performance Considerations

The changes only affect dependency resolution and shouldn't impact runtime performance. The on-change library is critical for state change detection in actors, so:

  • Monitor for any performance regressions in actor state tracking
  • The library is used at line 699 in actor/instance.ts for the #persist proxy - ensure this still performs well

Test Coverage

Recommendations:

  1. Add a test case that reproduces the ArrayBuffer error with the old version
  2. Verify all existing actor state tests pass
  3. Test hibernatable websocket state persistence (lines 1546-1650) since it involves ArrayBuffer operations

Suggested Improvements

  1. Add PR description:

    ## Problem
    Encountering "Method get ArrayBuffer.prototype.byteLength called on incompatible receiver" when using on-change with ArrayBuffer data.
    
    ## Solution
    Update to forked version @rivetkit/on-change@6.0.2-rc.1 which includes a fix for ArrayBuffer compatibility.
    
    ## Testing
    - [ ] Verified ArrayBuffer handling works correctly
    - [ ] All existing tests pass
    - [ ] Hibernatable WebSocket state persistence works
  2. Create a tracking issue for:

    • Promoting the fork to a stable version
    • Or contributing the fix upstream
    • Or documenting why a permanent fork is necessary
  3. Add a comment in package.json explaining the fork:

    "on-change": "npm:@rivetkit/on-change@^6.0.2-rc.1", // Custom fork with ArrayBuffer fix - see issue #XXXX

Approval Status

Conditional approval - the change appears functionally correct, but needs:

  1. Documentation (PR description)
  2. Confirmation that tests pass
  3. A plan for the RC version (when will it be stable?)

The technical implementation looks sound, but the lack of context and use of an RC version raises questions about production readiness.


Reviewed based on CLAUDE.md conventions and Rivet codebase patterns

@claude
Copy link

claude bot commented Nov 4, 2025

PR Review

Summary

This PR updates the on-change dependency from v5.0.1 to a custom fork @rivetkit/on-change@^6.0.2-rc.1 to fix an ArrayBuffer.prototype.byteLength error.

Code Quality ✅

Positive:

  • Clean dependency update with proper npm alias syntax
  • Minimal scope - only changes what's necessary
  • Uses semantic versioning with caret range for the RC version

Potential Issues ⚠️

  1. Missing Context: The PR body is empty. It would be helpful to include:

    • Steps to reproduce the original error
    • What specific use case triggers the ArrayBuffer.prototype.byteLength error
    • Why a custom fork is needed instead of using the official v6.x release
    • Link to the @rivetkit/on-change repository or explanation of the fork
  2. RC Version in Production: Using a release candidate (6.0.2-rc.1) raises concerns:

    • Is this intended to be temporary until a stable release?
    • Are there plans to migrate to an official release?
    • Consider adding a TODO or tracking issue for migrating to stable
  3. Custom Fork Maintenance: Using @rivetkit/on-change instead of the official package:

    • Adds maintenance burden for keeping the fork updated
    • May miss upstream bug fixes and improvements
    • What's the long-term plan for this fork?
  4. Test Coverage: The diff doesn't show any test additions:

    • Are there existing tests that verify this fix?
    • Can you add a test case that would fail with v5.0.1 and pass with v6.0.2-rc.1?

Security Considerations 🔒

  • The package integrity hash is present in the lockfile ✅
  • Using a custom package requires trust in the @rivetkit npm organization
  • Recommendation: Document what changes were made in the fork vs upstream

Testing Recommendations 🧪

  1. Run the full test suite to ensure no regressions:

    cd rivetkit-typescript/packages/rivetkit
    pnpm test
  2. Verify the specific ArrayBuffer use case works:

    • Test actor state persistence (where onChange is used in instance.ts:699)
    • Ensure state changes with ArrayBuffer data are tracked correctly
  3. Check if there are any integration tests that exercise the code path that was failing

Performance Considerations ⚡

  • The on-change library is used for state tracking in actors (instance.ts:699)
  • Version 6.x may have different performance characteristics than v5.x
  • Recommendation: Monitor performance metrics after deployment, especially for:
    • Actor state change detection
    • Memory usage with proxied objects
    • Hot paths in state mutations

Questions ❓

  1. What was the root cause of the ArrayBuffer.prototype.byteLength error?
  2. Is this a known issue in on-change v5.x, and if so, is it fixed in upstream v6.x?
  3. What specific changes are in the @rivetkit/on-change fork vs the official package?
  4. Is there a tracking issue for migrating off the RC version or fork?
  5. Have you tested this with production-like workloads?

Action Items 📋

Before merging:

  • Fill in PR description with context about the bug and fix
  • Verify tests pass with the new dependency
  • Document the fork's purpose and maintenance plan
  • Consider adding a test case for the ArrayBuffer scenario
  • Add a TODO or issue for migrating to stable/upstream if applicable

Verdict

The change itself is minimal and appears safe, but the PR lacks context needed for a full review. The use of a custom RC fork suggests this might be a temporary workaround, which should be documented clearly.

Recommendation: Request more information before merging, or merge if this is a critical hotfix and follow up with proper documentation.

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 4, 2025

Merge activity

  • Nov 4, 8:09 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 4, 8:11 PM UTC: CI is running for this pull request on a draft pull request (#3349) due to your merge queue CI optimization settings.
  • Nov 4, 8:41 PM UTC: The Graphite merge queue removed this pull request due to removal of a downstack PR #3342.
  • Nov 4, 8:49 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 4, 8:50 PM UTC: CI is running for this pull request on a draft pull request (#3352) due to your merge queue CI optimization settings.
  • Nov 4, 8:52 PM UTC: Merged by the Graphite merge queue via draft PR: #3352.

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025
@NathanFlurry NathanFlurry force-pushed the 11-02-fix_rivetkit_fix_method_get_arraybuffer.prototype.bytelength_called_on_incompatible_receiver_arraybuffer_ branch from 9d52114 to 84e9dc6 Compare November 4, 2025 20:25
@claude
Copy link

claude bot commented Nov 4, 2025

Pull Request Review

Summary

This PR upgrades the on-change dependency from v5.0.1 to a forked version @rivetkit/[email protected] to resolve an ArrayBuffer compatibility error: "Method get ArrayBuffer.prototype.byteLength called on incompatible receiver #".

Analysis

What the dependency does

The on-change library is used extensively in rivetkit-typescript/packages/rivetkit/src/actor/instance.ts to create proxy objects that automatically track state changes. Specifically, it's used on line 699 to wrap the persisted actor state (#persist) so that any modifications trigger automatic persistence and validation logic.

The fix

The error suggests that the older version (v5.0.1) of on-change had issues when proxying objects containing ArrayBuffers. Looking at the code, ArrayBuffers are used throughout for:

  • Scheduled event arguments (line 571-573)
  • WebSocket hibernation state (lines 1546-1633)
  • CBOR-encoded connection parameters and state (lines 2069-2098)

The upgrade to v6.0.2-rc.1 likely includes fixes for properly handling ArrayBuffer objects in the proxy system.

Code Quality: ✅ Good

Strengths:

  • Minimal, focused change addressing a specific runtime issue
  • Uses npm package aliasing correctly
  • Lock file changes are clean and consistent
  • No breaking changes to the API surface

Concerns

1. Using a Fork ⚠️ Major Concern

The change uses a @rivetkit scoped fork instead of the official on-change package. Questions:

  • Where is the fork hosted? Is it published from a trusted source?
  • What changes were made? Without a link to the fork's repository or changelog, it's impossible to audit what changed
  • Why not upstream? Has an issue been filed with the original on-change maintainer?
  • Long-term maintenance: Who will maintain this fork and keep it synced with upstream security updates?

Recommendation: Document the fork's purpose, repository URL, and migration plan in either:

  • A comment in package.json
  • The project's CLAUDE.md file
  • A docs/dependencies.md file

2. Release Candidate in Production ⚠️ Moderate Concern

Using a -rc.1 (release candidate) version suggests:

  • This may not be production-ready
  • There might be additional bugs or breaking changes before the final release
  • The version could be yanked from npm

Recommendation:

  • If this must ship, add monitoring to catch any related issues
  • Plan to upgrade to the stable v6.0.2 once available
  • Test thoroughly, especially the ArrayBuffer-heavy code paths (scheduled events, WebSocket hibernation)

3. Missing PR Context 📝 Minor Issue

The PR body is empty. Please add:

  • Reproduction steps for the original error
  • When/where the error occurred (dev, production, specific runtime?)
  • Testing performed to verify the fix
  • Link to upstream issue if one exists
  • Rollback plan if this causes regressions

4. Test Coverage ❓ Question

Looking at the file list, no test files were modified. Questions:

  • Have you verified existing tests pass?
  • Can you add a regression test that specifically exercises ArrayBuffer handling in actor state?
  • Test the specific scenarios: WebSocket hibernation, scheduled events with ArrayBuffer args

Security Considerations 🔒

Supply chain risk: Using a forked package from @rivetkit introduces trust dependencies:

  1. Verify the package:

    npm view @rivetkit/[email protected]

    Confirm it's published by the expected Rivet team member

  2. Pin the exact version: Consider changing from:

    "on-change": "npm:@rivetkit/on-change@^6.0.2-rc.1"

    to:

    "on-change": "npm:@rivetkit/[email protected]"

    This prevents automatic updates to untested RC versions

  3. Audit the package contents to ensure it matches expected changes

Performance Considerations

No performance concerns identified - this is a dependency update that should maintain the same performance characteristics.

Testing Recommendations

Please verify:

  1. Existing functionality:

    # Run rivetkit tests
    cd rivetkit-typescript/packages/rivetkit
    pnpm test
  2. ArrayBuffer-specific scenarios:

    • Actor state containing ArrayBuffers
    • Scheduled events with binary arguments
    • WebSocket hibernation (if applicable)
    • Connection state with CBOR-encoded binary data
  3. State change detection:
    Ensure the onChange callback (lines 699-760 in instance.ts) still properly detects:

    • Nested object changes
    • ArrayBuffer modifications
    • State validation and serialization

Recommendations Before Merge

  1. Add PR description with error details and testing notes
  2. ⚠️ Document the fork - add a comment in package.json explaining why
  3. ⚠️ Verify the package - confirm @rivetkit/on-change is from a trusted source
  4. Run full test suite and confirm no regressions
  5. 📝 Create tracking issue for migrating back to upstream on-change once a stable fix is available

Verdict

⚠️ Approve with Conditions

The change addresses a real runtime issue, but lacks documentation and raises questions about fork maintenance. I recommend merging once:

  1. PR description is added with testing details
  2. Fork source is verified and documented
  3. Full test suite passes
  4. A tracking issue is created for upstream migration

The technical implementation looks correct, but the process around using a forked RC needs better documentation for future maintainability.


Review completed following repository conventions from CLAUDE.md

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025
@graphite-app graphite-app bot closed this Nov 4, 2025
@graphite-app graphite-app bot deleted the 11-02-fix_rivetkit_fix_method_get_arraybuffer.prototype.bytelength_called_on_incompatible_receiver_arraybuffer_ branch November 4, 2025 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants